Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use the correct class for shared namespaces #754

Conversation

Gerst20051
Copy link
Contributor

@Gerst20051 Gerst20051 commented Aug 17, 2021

check commands to find the correct class when multiple classes have the same namespace

this is related to #246 and #247

I'm assuming the intention was to allow multiple classes to use the same namespace. I guess I don't understand the reasoning for the namespace if it can only be used for a single class.

🌈

@dorner
Copy link

dorner commented Aug 19, 2021

@Gerst20051 can you add a unit test for this?

@jb08
Copy link

jb08 commented Sep 20, 2021

I'm assuming the intention was to allow multiple classes to use the same namespace. I guess I don't understand the reasoning for the namespace if it can only be used for a single class.

+1

@hmistry
Copy link
Contributor

hmistry commented Jul 31, 2022

@Gerst20051 I couldn't push these updates to your fork+branch. Branch is ok to rebase to current Rails:Thor:main branch - no conflicts. Add this test and then PR is ready to merge. No updates to documentation needed as this is a bug fix.

Here is the test for this fix:

# File: spec/fixtures/script.thor
# add this at the bottom
class Apple < Thor
  namespace :fruits
 desc 'apple', 'apple'; def apple; end
end

class Pear < Thor
  namespace :fruits
 desc 'pear', 'pear'; def pear; end
end

# File: spec/util_spec.rb in L91
# add this test in the #find_class_and_command_by_namespace describe block
  describe "#find_class_and_command_by_namespace" do
    # .... existing tests
    it "returns correct Thor class and the command name when shared namespaces" do
      expect(Thor::Util.find_class_and_command_by_namespace("fruits:apple")).to eq([Apple, "apple"])
      expect(Thor::Util.find_class_and_command_by_namespace("fruits:pear")).to eq([Pear, "pear"])
    end
  end

# File: spec/base_spec.rb in L193
# add `Apple`, `Pear` to the expected list
    it "returns tracked subclasses, grouped by the files they come from" do
      thorfile = File.join(File.dirname(__FILE__), "fixtures", "script.thor")
      expect(Thor::Base.subclass_files[File.expand_path(thorfile)]).to eq([
        MyScript, MyScript::AnotherScript, MyChildScript, Barn,
        PackageNameScript, Scripts::MyScript, Scripts::MyDefaults,
        Scripts::ChildDefault, Scripts::Arities, Apple, Pear
      ])
    end

LGTM 🚀

@hmistry hmistry force-pushed the gerst/fix-multiple-classes-with-same-namespace-bug branch from f9499ef to 1d34fa5 Compare July 31, 2022 21:12
@hmistry
Copy link
Contributor

hmistry commented Jul 31, 2022

@dorner Add unit tests, please review... LGTM

@Gerst20051 Gerst20051 changed the title check commands to find the correct class when multiple classes have t… use the correct class for shared namespaces Aug 1, 2022
Copy link

@dorner dorner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me! @rafaelfranca ?

@hmistry
Copy link
Contributor

hmistry commented Aug 9, 2022

Hi @rafaelfranca, @dorner, @deivid-rodriguez. Any chance this can be reviewed soon and merged so it's in the next release? This bug impacts our ability to organize thor tasks in our project into manageable grouped namespaces.

@Gerst20051
Copy link
Contributor Author

@dorner can you help move this along? how should we proceed to get this released?

@dorner
Copy link

dorner commented Jan 25, 2023

Unfortunately I'm not an admin here - @rafaelfranca is the closest thing we have to an active admin and he's sadly pretty busy. :(

We only need to find the class by namespace and command in one place,
so just move that logic there.
@rafaelfranca rafaelfranca merged commit 9f41cfb into rails:main May 11, 2023
@hmistry
Copy link
Contributor

hmistry commented May 14, 2023

@rafaelfranca Thank you for merging this in and refactoring it. We look forward to seeing it in the next release and start using namespaces across classes. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants